Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix setting cursor position in send keys #1686

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fix setting cursor position in send keys #1686

wants to merge 1 commit into from

Conversation

jgraham
Copy link
Member

@jgraham jgraham commented Sep 24, 2022 •
edited by pr-preview bot
Loading

I believe the expected behaviour is:

  • If the target already has focus, do nothing
  • Otherwise, if the target is focusable, set the selection to the last editable point in the element.

Previously the spec was unconditionally focusing the target element (execpting some cases with file uploads), and therefore the letter of the spec was to never move the caret.

This change updates it to only focus the target if it's not already the active element, and then move the caret to the end.

Preview | Diff

@jgraham
Fix setting cursor position in send keys
8857c92 
I believe the expected behaviour is:
* If the target already has focus, do nothing
* Otherwise, if the target is focusable, set the selection to the last
editable point in the element.

Previously the spec was unconditionally focusing the target
element (execpting some cases with file uploads), and therefore the
letter of the spec was to never move the caret.

This change updates it to only focus the target if it's not already
the active element, and then move the caret to the end.
@jgraham
Copy link
Member Author

jgraham commented Sep 24, 2022

An open question is what to do if a child of the target already has focus e.g. if the <div> is the target and the <span> is the activeElement in

<div contenteditable>
foo
<span>bar</span>
baz
</div>

Currently this will set the focus to the end of the <div>.

@jgraham
Copy link
Member Author

jgraham commented Sep 24, 2022

I think I'm convinced that the child element case is reasonable for contenteditable, but what about <select>/<option>. If you sendKeys to an option element should it focus / scrollIntoView the select? I'm not quite sure if an <option> is considered to have a focusable area, but maybe we just want to always use the <select> in that case? There seem to be some existing tests that assume we'll do so.

@whimboo
Copy link
Contributor

whimboo commented Oct 4, 2022 •
edited
Loading

Please note that there is an upstream sync PR from @jgraham open which covers that proposed change and might merge soon: web-platform-tests/wpt#36226

@whimboo whimboo requested a review from shs96c October 4, 2022 09:27
@whimboo
Copy link
Contributor

whimboo commented Oct 4, 2022

@sadym-chromium maybe you could review as well?

@jgraham
Copy link
Member Author

jgraham commented Oct 4, 2022

I ended up moving the difficult <option> cases out of wpt, because they were testing form control rendering in a way that doesn't make sense cross browser/platform.

sadym-chromium
sadym-chromium reviewed Oct 4, 2022
View reviewed changes
Copy link
Contributor

@sadym-chromium sadym-chromium left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit confused by the "content editable" option.

  1. It looks like the behaviour changed: It used to set a cursor anywhere inside the element, and now it selects the whole content. Was it intended?
  2. (out of scope of the this PR) Probably I'm missing something. How do users suppose to change the value of the "content editable" element?

@whimboo
Copy link
Contributor

whimboo commented Oct 25, 2022

@jgraham would you mind checking the feedback as received from @sadym-chromium?

@jgraham
Copy link
Member Author

jgraham commented Oct 25, 2022 •
edited
Loading

  1. It looks like the behaviour changed: It used to set a cursor anywhere inside the element, and now it selects the whole content. Was it intended?

Unless I've mae a mistake with the spec (very possible, but you'll need to be more specific if so), it puts the cursor at the end (last editable point) of the element, which I believe was the intended behavior from the start.

  1. (out of scope of the this PR) Probably I'm missing something. How do users suppose to change the value of the "content editable" element?

By inserting text into it? I don't really understand the question, but in general if you want to do arbitary updates to a contenteditable element, then you need to use the actions API.

sadym-chromium
sadym-chromium reviewed Nov 3, 2022
View reviewed changes
index.html
Comment on lines +6306 to +6307
<li><p>Set the <a>active document</a>'s <a>selection</a>
to <var>range</var>.
Copy link
Contributor

@sadym-chromium sadym-chromium Nov 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if this means to select the whole element's content, or put just to put selection to the very end?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's supposed to be a collapsed range, whose start and end node is element and whose offset is child count i.e. a zero-length selection corresponding to a caret position at the end of the element.

@sadym-chromium
Copy link
Contributor

  1. It looks like the behaviour changed: It used to set a cursor anywhere inside the element, and now it selects the whole content. Was it intended?

Unless I've mae a mistake with the spec (very possible, but you'll need to be more specific if so), it puts the cursor at the end (last editable point) of the element, which I believe was the intended behavior from the start.

Added the comment in the code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sadym-chromium sadym-chromium sadym-chromium left review comments

@shs96c shs96c Awaiting requested review from shs96c

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
bug Element Send Keys needs-discussion
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jgraham @whimboo @sadym-chromium